I had to limit the types used by tsc, because I had conflicts in @types/react-native such as
node_modules/@types/react-native/globals.d.ts (36,15): Duplicate identifier 'FormData'.
See this discussion DefinitelyTyped/DefinitelyTyped#15960

@aduth, @dsifford or @sirreal – can you do sanity check?

I'd use separate tsconfig files for the browser and native files... Separate them with include and/or exclude properties.

More specifically, I'd keep the main tsconfig.json as-is, and add in a tsconfig.native.json for the native stuff and then just build the project in series.

A better option if you aren't set on the structure is to drop all react-native stuff somewhere with a common parent directory (e.g., packages/react-native/aztec/xxx, packages/react-native/bridge/xxx) and then just drop a tsconfig at packages/react-native/tsconfig.json and let typescript do the rest.

I think the main issue here is that tsc loads everything installed in node_modules/@types by default. And since we added react-native as a dependency, new types are now installed with it that are causing conflicts in the main (web) build. So splitting the tsconfig or adding a new one in our rn package won't address this problem

Which type definitions are conflicting? Sounds like they are defining ambients that are conflicting with node's ambients? There must be a situation where this has affected some other projects as well, no?

I'd be extremely cautious in specifying individual types by name because that could lead to a situation where, if someone were to install a new set of definitions without also including the name of those in the tsconfig, the bug would be difficult to pinpoint.

Additionally, I'm not sure how this would play with definitions that depend on other definitions. You would have to include those as well in your list of types. (i.e., this doesn't scale particularly well).

Suggestions here @DanielRosenwasser?

Yes I have found this issue reported elsewhere online such as on DefinitelyTyped (see first comment).
Here's the full report from running tsc without this line in tsconfig.json:

node_modules/@types/react-native/globals.d.ts:40:15 - error TS2300: Duplicate identifier 'FormData'.

40 declare class FormData {
                 ~~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:5353:11
    5353 interface FormData {
                   ~~~~~~~~
    'FormData' was also declared here.
  node_modules/typescript/lib/lib.dom.d.ts:5363:13
    5363 declare var FormData: {
                     ~~~~~~~~
    and here.

node_modules/@types/react-native/globals.d.ts:85:5 - error TS2717: Subsequent property declarations must have the same type.  Property 'body' must be of type 'string | ArrayBuffer | ArrayBufferView | Blob | FormData | URLSearchParams | ReadableStream<Uint8Array> | null | undefined', but here has type 'string | ArrayBuffer | DataView | Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array | Blob | FormData | null | undefined'.

85     body?: BodyInit_;
       ~~~~

  node_modules/typescript/lib/lib.dom.d.ts:1413:5
    1413     body?: BodyInit | null;
             ~~~~
    'body' was also declared here.

node_modules/@types/react-native/globals.d.ts:111:14 - error TS2300: Duplicate identifier 'RequestInfo'.

111 declare type RequestInfo = Request | string;
                 ~~~~~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:18568:6
    18568 type RequestInfo = Request | string;
               ~~~~~~~~~~~
    'RequestInfo' was also declared here.

node_modules/@types/react-native/globals.d.ts:130:13 - error TS2403: Subsequent variable declarations must have the same type.  Variable 'Response' must be of type '{ new (body?: string | ArrayBuffer | ArrayBufferView | Blob | FormData | URLSearchParams | ReadableStream<Uint8Array> | null | undefined, init?: ResponseInit | undefined): Response; prototype: Response; error(): Response; redirect(url: string, status?: number | undefined): Response; }', but here has type '{ new (body?: string | ArrayBuffer | DataView | Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array | Blob | FormData | null | undefined, init?: ResponseInit | undefined): Response; prototype: Response; error: () => Response; redirect: (url: ...'.

130 declare var Response: {
                ~~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:12463:13
    12463 declare var Response: {
                      ~~~~~~~~
    'Response' was also declared here.

node_modules/@types/react-native/globals.d.ts:253:14 - error TS2300: Duplicate identifier 'XMLHttpRequestResponseType'.

253 declare type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:18746:6
    18746 type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
               ~~~~~~~~~~~~~~~~~~~~~~~~~~
    'XMLHttpRequestResponseType' was also declared here.

node_modules/@types/react-native/index.d.ts:9417:18 - error TS2717: Subsequent property declarations must have the same type.  Property 'geolocation' must be of type 'Geolocation', but here has type 'GeolocationStatic'.

9417         readonly geolocation: Geolocation;
                      ~~~~~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:10581:14
    10581     readonly geolocation: Geolocation;
                       ~~~~~~~~~~~
    'geolocation' was also declared here.

node_modules/@types/react-native/index.d.ts:9420:11 - error TS2451: Cannot redeclare block-scoped variable 'navigator'.

9420     const navigator: Navigator;
               ~~~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:18152:13
    18152 declare var navigator: Navigator;
                      ~~~~~~~~~
    'navigator' was also declared here.

node_modules/typescript/lib/lib.dom.d.ts:5353:11 - error TS2300: Duplicate identifier 'FormData'.

5353 interface FormData {
               ~~~~~~~~

  node_modules/@types/react-native/globals.d.ts:40:15
    40 declare class FormData {
                     ~~~~~~~~
    'FormData' was also declared here.

node_modules/typescript/lib/lib.dom.d.ts:5363:13 - error TS2300: Duplicate identifier 'FormData'.

5363 declare var FormData: {
                 ~~~~~~~~

  node_modules/@types/react-native/globals.d.ts:40:15
    40 declare class FormData {
                     ~~~~~~~~
    'FormData' was also declared here.

node_modules/typescript/lib/lib.dom.d.ts:18152:13 - error TS2451: Cannot redeclare block-scoped variable 'navigator'.

18152 declare var navigator: Navigator;
                  ~~~~~~~~~

  node_modules/@types/react-native/index.d.ts:9420:11
    9420     const navigator: Navigator;
                   ~~~~~~~~~
    'navigator' was also declared here.

node_modules/typescript/lib/lib.dom.d.ts:18568:6 - error TS2300: Duplicate identifier 'RequestInfo'.

18568 type RequestInfo = Request | string;
           ~~~~~~~~~~~

  node_modules/@types/react-native/globals.d.ts:111:14
    111 declare type RequestInfo = Request | string;
                     ~~~~~~~~~~~
    'RequestInfo' was also declared here.

node_modules/typescript/lib/lib.dom.d.ts:18746:6 - error TS2300: Duplicate identifier 'XMLHttpRequestResponseType'.

18746 type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
           ~~~~~~~~~~~~~~~~~~~~~~~~~~

  node_modules/@types/react-native/globals.d.ts:253:14
    253 declare type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~
    'XMLHttpRequestResponseType' was also declared here.


Found 12 errors.

My only other thought before hearing from Daniel is maybe adding the @types/react-native path to exclude for the browser tsconfig would work if the consensus was to split into separate tsconfig files.

I'd use separate tsconfig files for the browser and native files... Separate them with include and/or exclude properties.

I agree, and I'm more and more convinced that we'll want separate tsconfig files by package. #18942 is one example, and I think we'll see the advantages of fine-grained control over time.

It would be great to see that with test files and jest as well. Being able to grab global describe or it anywhere in the project is a potential source of bugs.

Which type definitions are conflicting? Sounds like they are defining ambients that are conflicting with node's ambients? There must be a situation where this has affected some other projects as well, no?

I've experienced conflicts in another monorepo, and my conclusion was that explicitly defining types becomes necessary. In the monorepo setups I'm familiar with —this one and Calypso— @types are devDependencies, and devDependencies are always root dependencies.

When building multiple packages that target different platforms, it's not surprising that including all root @types in all builds leads to problems and conflicts. The browser, node, and react-native are different platforms and different and conflicting types depending on our target platform seems desirable.

I'd be extremely cautious in specifying individual types by name because that could lead to a situation where, if someone were to install a new set of definitions without also including the name of those in the tsconfig, the bug would be difficult to pinpoint.

Additionally, I'm not sure how this would play with definitions that depend on other definitions. You would have to include those as well in your list of types. (i.e., this doesn't scale particularly well).

My (limited) experience has been that few type declarations are ambient/global, and they should be treated with care. Most package types will be included by explicitly importing the associated module.

I'm a bit surprised by the inclusion of react in this list. Maybe a simpler and more portable JSX import could be used? I'd expect react imports to be included when including @wordpress/element.

I've excluded react-native packages from this tsconfig, eventually we'll have our own tsconfig as we port our code from flow 👍
In the meantime, are we ok to keep those types in? I did not want to introduce a new JSX type here as @types/react is already installed as a dependency but I'm happy to help revisit this in another PR

I haven't converted flow files to esnext yet but I just wanted to have less dependencies to deal with

A similar question, how do you plan to work with this package given that there is a huge number of scripts? Some of them reference the local node_modules folder but others go two levels up.

I'm not sure yet, that's something we can discuss here or on follow up PRs when we tackle this problem. I think that given the amount of scripts we might want to keep those here for now as to not clutter the main package.json.

Some of them reference the local node_modules folder but others go two levels up.

In theory there should not be any local node_modules folder, since that indicates a conflict. We'll work to fix those conflicts.

@aduth, @dsifford or @sirreal – can you do sanity check?

In the meantime, @youknowriad introduced a list of bundled packages that don't get included:

Closer to merge, we should consolidate it somehow.

Should react be listed here as well? I see it listed in another package. Given that RN depends on React, we probably can remove it on another package, what do you think?

I'm not sure to be honest, we haven't used or thought about react-native-bridge as a standalone package. It's still tightly coupled with the main native editor, thus the reason why I replaced the version with "*" here, so we can revisit later.
We could indeed add react here, but I wonder if it would change anything in termes of packages that use this bridge dependency, since react is already a peer dependency of react-native it should display an error or a warning if react is not a dependency.

Indeed react is a dependency of react-native-aztec, so it could probably be removed there. Let's revisit peerDependencies later if that's ok with you?

Sure thing, I just flagged items to go through during the final iteration 👍